That's a good start, definitely needs a test.

Which file would be appropriate for its test? I am confused between test_distributions.py and test_distributions_random.py

I would add a test to https://github.com/pymc-devs/pymc3/blob/718dab9ded5bf99d2fcfcd4ec8e2cf9769bb216a/pymc3/tests/test_distributions_random.py where you create a DensityDist for the Normal distribution (so the random function is trivial) and test it like we do the Normal random method.

@twiecki Could you please provide the link to the gist or line number of the test for Normal random method you mentioned, I couldn't find it?

@Vaibhavdixit02 https://github.com/pymc-devs/pymc3/blob/718dab9ded5bf99d2fcfcd4ec8e2cf9769bb216a/pymc3/tests/test_distributions_random.py#L379

def test_densitydist(self):
        def ref_rand(size, mu, sd):
            return st.norm.rvs(size=size, loc=mu, scale=sd)
        normal_dist = pm.Normal.dist()
        pymc3_random(pm.DensityDist, {'logp':normal_dist.logp, 'random':normal_dist.random}, ref_rand=ref_rand)

This is the test I came up with, but it is throwing error when I run it locally. The error I got is,

distfam = <class 'pymc3.distributions.distribution.DensityDist'>, valuedomain = <pymc3.tests.test_distributions.Domain object at 0x7f4781f738d0>
vardomains = {'logp': <bound method Normal.logp of <pymc3.distributions.continuous.Normal object at 0x7f47125d3790>>, 'random': <bound method Normal.random of <pymc3.distributions.continuous.Normal object at 0x7f47125d3790>>}
extra_args = {}

    def build_model(distfam, valuedomain, vardomains, extra_args=None):
        if extra_args is None:
            extra_args = {}
        with Model() as m:
            vals = {}
            for v, dom in vardomains.items():
               vals[v] = Flat(v, dtype=dom.dtype, shape=dom.shape,
                               testval=dom.vals[0])
               AttributeError: 'function' object has no attribute 'dtype'

test_distributions.py:149: AttributeError

The problem is in the passing of functions (for logp and random) as parameters in the dictionary, I can't figure out any workaround for this can you help me out on this @twiecki?

This is a bit more tricky than I thought. This is how far I got:

    def custom_random(self, point=None, size=None, repeat=None):
        mu, tau, _ = draw_values([self.mu, self.tau, self.sd],
                                 point=point)
        return generate_samples(stats.norm.rvs, loc=mu, scale=tau**-0.5,
                                dist_shape=self.shape,
                                size=size)

    def test_density_dist(self):
        def ref_rand(size, mu, sd):
            return st.norm.rvs(size=size, loc=mu, scale=sd)
        def create_custom_dens(name, mu=0, sd=1, shape=None, transform=None):
            return pm.DensityDist(name, lambda value: np.log(st.norm(loc=mu, scale=sd).pdf(value)), #logp
                random=custom_random)
        pymc3_random(create_custom_dens, {'mu': R, 'sd': Rplus}, ref_rand=ref_rand)

A couple of notes:

• Seems like the test wants to be passed a class and create the RV itself (https://github.com/pymc-devs/pymc3/blob/718dab9ded5bf99d2fcfcd4ec8e2cf9769bb216a/pymc3/tests/test_distributions.py#L152). Here I'm passing a function that returns an object in the hope that it accepts it as well.
• The random function also has a slightly more complex call signature. I copy&pasted the random method from pm.Normal and pass it into the created DensityDist but clearly this won't work as written. I would suggest to drop in there with a debugger like pdb++ (highly recommended to learn using this if you haven't already) and examine the arguments and how to return the right random numbers.

Hopefully that gets us a step closer.

Thank you for such a detailed reply, I will try what you have suggested and get back to you once I have some working version ready.

@twiecki are you familiar with any other similar test that could serve as a reference? My efforts have not lead to anything, I think looking at some example would be helpful.

@Vaibhavdixit02 implementing test could be tricky, you might find this PR useful: #2443
(As you can see from the discussion, I got stuck implementing the test at some point as well. You can have a look at the commit history, which might help as well)

Along the lines of the test for LKJCorr I tried.

    def test_density_dist(self):
        def ref_rand(size, mu, sd):
            return st.norm.rvs(size=size, loc=mu, scale=sd)
        
        class TestDensityDist(pm.DensityDist):

            def __init__(self, **kwargs):
                norm_dist = pm.Normal.dist()
                super(TestDensityDist, self).__init__('normal', logp=norm_dist.logp, random=norm_dist.random)

        pymc3_random(TestDensityDist, {},ref_rand=ref_rand)

But I am getting

    def __init__(self, **kwargs):
        norm_dist = pm.Normal.dist()
>       super(TestDensityDist, self).__init__('normal', logp=norm_dist.logp, random=norm_dist.random)
E       TypeError: __init__() got multiple values for keyword argument 'logp'

test_distributions_random.py:684: TypeError

Can't figure out why it says multiple arguments for logp

Because the first argument is already the logp (https://github.com/pymc-devs/pymc3/pull/2805/files#diff-eb25725602c01ec0a384e4b6d3946331R181). You can just drop the 'normal' arg.

This test was passing locally for me, please tell me if there is some conceptual error. 🙂

That looks great! Would it be easy from here to do the same put calling pm.DensityDist() directly?

The problem with pm.DensityDist() is that without a model instance calling it is not possible (like all other distributions) to instantiate it and my attempts of passing it without instantiating it failed as we need to pass the logp and random from the pm.Normal.dist() object.

Oh it needs a model object on the stack? I suppose we could write a separate test that doesn't use the pymc3_random helper function and only tests that sampling works at all (i.e. not check the resulting distribution which is covered by this test). Sorry to be dense on this but I think we really should have a test in here that tests how a user would use it.

I think that would be the simplest and most straightforward approach, to create a separate model instance for DensityDist I am not sure how checking if the sampling is working fine would be implemented but over all this is the simpler approach.

@Vaibhavdixit02 The test I'm imagining would not test that sampling is valid. It would literarily just be a function that creates a model context, creates a DensityDist with random, and then does ppc and makes sure there is no exception and the len(ppc_samples) > 0. Correctness is already established by the test you have now.

Okay sure. I think we could use the normal logp and random method in this test too?

@twiecki have added that test. 🙂

This is shaping up nicely. See minor comments to resolve as well as the bad merge. Also, please add this (with credit) to the release-notes.

@twiecki added the test with scipy's normal distribution, the test passed locally but I am not very sure if it is correct, please provide feedback if it needs to be changed.

This looks great, thanks for figuring this out!

Only need to add this to the release notes.

Should I add it to the release notes or will the maintainers do that?

Congrats @Vaibhavdixit02!

Yay! 😄

@Vaibhavdixit02 Sorry, I realized two more things here (see new comments). Could you also address those in a separate PR?